Change tool metadata file format to JSON #1553

vicroms · 2024-12-13T08:04:55Z

Continuation of #1490
Related: #1277

This PR contains several changes:

Moves vcpkgTools.xml data into a JSON file and removes the XML parsing code.
Adds the architecture field to the tool metadata.
Removes the requirement of force system binaries on arm64 Linux platforms.

To do:

Update Ninja version to one with ARM64 support.
Test with full registry build.

…ting trivial destruction.

…the search loop, and get rid of magic numbers that is the length of the table.

Make the table be static data, deduplicate part of the expression in the search loop, and get rid of magic numbers that is the length of the table.

src/vcpkg/tools.cpp

BillyONeal

Thanks for the feature!

BillyONeal · 2024-12-23T14:52:06Z

src/vcpkg/base/json.cpp

+        if (Util::contains(KNOWN_ARCHITECTURES, sv))
+        {
+            return sv.to_string();
+        }
+


Suggested change

if (Util::contains(KNOWN_ARCHITECTURES, sv))

{

return sv.to_string();

}

if (to_cpu_architecture(sv).has_value()) { return sv.to_string(); }

?

Alternately, can this just be made to return CPUArchitecture directly instead? This also avoids the need to have a duplicate KNOWN_ARCHITECTURES table.

(If you still want the table to generate the error message, can it at least be combined with the bits powering to_cpu_architecture so that we don't introduce a bug any time any of those are edited? For example:

struct KnownCPUArchitecture { StringLiteral name; CPUArchitecture architecture; }; inline constexpr KnownCPUArchitecture KnownArchitectures[] = { {"x86", CPUArchitecture::X86}, {"x64", CPUArchitecture::X64}, {"amd64", CPUArchitecture::X64}, {"arm", CPUArchitecture::ARM}, {"arm64", CPUArchitecture::ARM64}, {"arm64ec", CPUArchitecture::ARM64EC}, {"s390x", CPUArchitecture::S390X}, {"ppc64le", CPUArchitecture::PPC64LE}, {"riscv32", CPUArchitecture::RISCV32}, {"riscv64", CPUArchitecture::RISCV64}, {"loongarch32", CPUArchitecture::LOONGARCH32}, {"loongarch64", CPUArchitecture::LOONGARCH64}, {"mips64", CPUArchitecture::MIPS64}, };

)

BillyONeal · 2024-12-23T14:55:57Z

src/vcpkg/vcpkgcmdarguments.cpp

@@ -358,6 +358,7 @@ namespace vcpkg
        args.parser.parse_option(
            SwitchBuiltinRegistryVersionsDir, StabilityTag::Experimental, args.builtin_registry_versions_dir);
        args.parser.parse_option(SwitchRegistriesCache, StabilityTag::Experimental, args.registries_cache_dir);
+        args.parser.parse_option(SwitchToolDataFile, StabilityTag::ImplementationDetail, args.tools_data_file);


Don't forget docs :)

BillyONeal · 2024-12-23T14:57:31Z

include/vcpkg/tools.test.h

@@ -1,6 +1,7 @@
 #pragma once


I really think this should just be in tools.h given that the things in here are contractual despite trying to claim that it's used for 'test'ing. But that's a preexisting situation so no change requested if you don't want to go there.

BillyONeal · 2024-12-23T14:58:03Z

src/vcpkg-test/tools.cpp

+    REQUIRE(!invalid_arch.has_value());
+    CHECK("invalid_arch.json: error: $.tools[0].arch (a CPU architecture): Invalid architecture: notanarchitecture. "
+          "Expected "
+          "one of: x86,x64,arm,arm64,arm64ec,s390x,ppc64le,riscv32,riscv64,loongarch32,loongarch64,mips64\n"


Suggested change

"one of: x86,x64,arm,arm64,arm64ec,s390x,ppc64le,riscv32,riscv64,loongarch32,loongarch64,mips64\n"

"one of: x86, x64,arm, arm64, arm64ec, s390x, ppc64le, riscv32, riscv64, loongarch32, loongarch64,mips64\n"

Suggested change

"one of: x86,x64,arm,arm64,arm64ec,s390x,ppc64le,riscv32,riscv64,loongarch32,loongarch64,mips64\n"

"one of: x86, x64, arm, arm64, arm64ec, s390x, ppc64le, riscv32, riscv64, loongarch32, loongarch64, mips64\n"

BillyONeal · 2024-12-23T15:00:48Z

src/vcpkg/tools.cpp

+        if (auto tool_data = maybe_tool_data.get())
+        {
+            return *tool_data;
+        }
+
+        return msg::format(msgErrorWhileParsingToolData, msg::path = origin);


Suggested change

if (auto tool_data = maybe_tool_data.get())

{

return *tool_data;

}

return msg::format(msgErrorWhileParsingToolData, msg::path = origin);

return maybe_tool_data.value_or_exit(VCPKG_LINE_INFO);

Any errors in parsing should have been reported by Json::Reader, so if we get here vcpkg has a bug and we shouldn't have separate localized messages for bugs.

BillyONeal · 2024-12-23T15:02:35Z

src/vcpkg/tools.cpp

+        auto as_json = Json::parse(contents, origin);
+        if (!as_json)
+        {
+            return as_json.error();
+        }
+        auto as_value = std::move(as_json).value(VCPKG_LINE_INFO).value;


This should have error handling like that in Json::parse_object rather than crashing if it can't be interpreted as a value here.

BillyONeal · 2024-12-23T15:05:16Z

src/vcpkg/tools.cpp

+        std::error_code ec;
+        auto contents = fs.read_contents(path, ec);
+        if (ec)
+        {
+            return format_filesystem_call_error(ec, __func__, {path});
+        }
+
+        return parse_tool_data(contents, path);


Suggested change

std::error_code ec;

auto contents = fs.read_contents(path, ec);

if (ec)

{

return format_filesystem_call_error(ec, __func__, {path});

}

return parse_tool_data(contents, path);

return fs.try_read_contents(path).then([](FileContents&& fc) -> ExpectedL<std::vector<ToolDataEntry>> {

return parse_tool_data(fc.content, Path{fc.origin});

});

BillyONeal · 2024-12-23T15:06:19Z

src/vcpkg/tools.cpp

+            std::string arch_str;
+            if (r.optional_object_field(obj, "arch", arch_str, Json::ArchitectureDeserializer::instance))
+            {
+                auto it = arch_map.find(arch_str);


Can you use to_cpu_architecture instead of a second duplicate table?

autoantwort

Can you also add a json schema for this file?

autoantwort · 2024-12-28T22:12:28Z

azure-pipelines/end-to-end-tests-dir/fetch.ps1

+        "name": "cmake",
+        "os": "linux",
+        "version": "3.22.2",
+        "executable": "cmake-3.22.2-linux-x86_64/bin/cmake",
+        "url": "https://github.com/Kitware/CMake/releases/download/v3.22.2/cmake-3.22.2-linux-x86_64.tar.gz",
+        "sha512": "579e08b086f6903ef063697fca1dc2692f68a7341dd35998990b772b4221cdb5b1deecfa73bad9d46817ef09e58882b2adff9d64f959c01002c11448a878746b",
+        "archive": "cmake-3.22.2linux-x86_64.tar.gz"


Since this entry has no architecture entry this version would be used on arm64 devices. Maybe the arch field should be mandatory.

data-queue and others added 15 commits September 12, 2024 04:34

Move vcpkgTools.xml into the tool

917e69e

fix

23c303e

update

e86a63b

format

65511af

reviews

e6ce6bb

fix

db4bf03

Allow constexpr Optional<T> when constexpr T is acceptable by propaga…

3722ced

…ting trivial destruction.

Make the table be static data, deduplicate part of the expression in …

7126108

…the search loop, and get rid of magic numbers that is the length of the table.

Fixup optional 😅

c340da3

clang-format

6c41ed5

Merge pull request microsoft#1 from BillyONeal/tools

107eec5

Make the table be static data, deduplicate part of the expression in the search loop, and get rid of magic numbers that is the length of the table.

Suppress -Werror=unused-but-set-variable in optional tests.

7e6947c

fix

8b5511c

fix

b183ea6

Merge remote-tracking branch 'origin/main' into tools

6defa28

vicroms force-pushed the tools branch 2 times, most recently from 5fe30dc to 3747fae Compare December 13, 2024 09:46

wip

4d8eb9b

vicroms force-pushed the tools branch from 3747fae to 4d8eb9b Compare December 13, 2024 09:59

vicroms added 2 commits December 13, 2024 02:25

Add deserializer types and errors

80cc2ab

more error messages

3b33ef6

vicroms changed the title ~~[WIP] Add ARM64 Linux tools~~ [WIP] Change tool metadata file format to JSON Dec 13, 2024

Add parser unit tests

b7075b8

vicroms commented Dec 14, 2024

View reviewed changes

src/vcpkg/tools.cpp Show resolved Hide resolved

vicroms marked this pull request as ready for review December 14, 2024 22:10

vicroms mentioned this pull request Dec 14, 2024

Move vcpkgTools.xml into the tool and better support for arm64-linux #1490

Closed

Add schema-version

d1cf8fb

vicroms changed the title ~~[WIP] Change tool metadata file format to JSON~~ Change tool metadata file format to JSON Dec 16, 2024

vicroms and others added 2 commits December 16, 2024 16:59

Update vcpkg-scripts-sha.txt

61a8b34

Merge branch 'main' into tools

77dc31b

vicroms added 6 commits December 17, 2024 01:27

Fix message error

4720537

Merge github.com:vicroms/vcpkg-tool into tools

9091bd9

Merge branch 'tools' of github.com:vicroms/vcpkg-tool into tools

2c5515a

Fix format errors

20ad083

generate message map

86b54d0

Add redirection switch

3b7bb19

BillyONeal approved these changes Dec 23, 2024

View reviewed changes

autoantwort reviewed Dec 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change tool metadata file format to JSON #1553

Change tool metadata file format to JSON #1553

vicroms commented Dec 13, 2024 •

edited

Loading

BillyONeal left a comment

BillyONeal Dec 23, 2024

BillyONeal Dec 23, 2024

BillyONeal Dec 23, 2024

BillyONeal Dec 23, 2024

dg0yt Dec 29, 2024

BillyONeal Dec 23, 2024

BillyONeal Dec 23, 2024

BillyONeal Dec 23, 2024

BillyONeal Dec 23, 2024

autoantwort left a comment

autoantwort Dec 28, 2024

	"one of: x86,x64,arm,arm64,arm64ec,s390x,ppc64le,riscv32,riscv64,loongarch32,loongarch64,mips64\n"
	"one of: x86, x64,arm, arm64, arm64ec, s390x, ppc64le, riscv32, riscv64, loongarch32, loongarch64,mips64\n"

	"one of: x86,x64,arm,arm64,arm64ec,s390x,ppc64le,riscv32,riscv64,loongarch32,loongarch64,mips64\n"
	"one of: x86, x64, arm, arm64, arm64ec, s390x, ppc64le, riscv32, riscv64, loongarch32, loongarch64, mips64\n"

Change tool metadata file format to JSON #1553

Are you sure you want to change the base?

Change tool metadata file format to JSON #1553

Conversation

vicroms commented Dec 13, 2024 • edited Loading

BillyONeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autoantwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicroms commented Dec 13, 2024 •

edited

Loading